Implement partials compilation #2212

tstirrat15 · 2025-01-28T14:37:06Z

Description

This implements partials support in the composable schema language.

Changes

Gonna annotate.

Testing

Review. See that tests pass. Suggest any missing tests.

tstirrat15

See annotations

tstirrat15 · 2025-01-30T22:40:23Z

go.mod

@@ -1,6 +1,6 @@
 module github.com/authzed/spicedb

-go 1.23.1
+go 1.23.5


This may not actually be necessary; I changed it when I thought it was a fix for vulnerability issues. I can revert if desired.

tstirrat15 · 2025-01-30T23:05:57Z

pkg/composableschemadsl/compiler/compiler.go

-type compilationContext struct {
-	// The set of definition names that we've seen as we compile.
-	// If these collide we throw an error.
-	existingNames *mapz.Set[string]
-	// The global set of files we've visited in the import process.
-	// If these collide we short circuit, preventing duplicate imports.
-	globallyVisitedFiles *mapz.Set[string]
-	// The set of files that we've visited on a particular leg of the recursion.
-	// This allows for detection of circular imports.
-	// NOTE: This depends on an assumption that a depth-first search will always
-	// find a cycle, even if we're otherwise marking globally visited nodes.
-	locallyVisitedFiles *mapz.Set[string]
-}


These were previously values that we passed through the tree, because the import logic invoked compilation. That isn't possible with partials since a partial can be defined in one file and used in another, so among other changes, this PR makes the import logic operate on the DSL tree as an intermediate step.

tstirrat15 · 2025-01-30T23:06:22Z

pkg/composableschemadsl/compiler/compiler.go

-	return compileImpl(schema, cctx, prefix, opts...)
-}
-
-func compileImpl(schema InputSchema, cctx compilationContext, prefix ObjectPrefixOption, opts ...Option) (*CompiledSchema, error) {


Flattening these since there's no longer an invocation in the import logic

tstirrat15 · 2025-01-30T23:07:23Z

pkg/composableschemadsl/compiler/compiler.go

-		skipValidate:         cfg.skipValidation,
+	// NOTE: import translation is done separately so that partial references
+	// and definitions defined in separate files can correctly resolve.
+	err = translateImports(importTranslationContext{


Note that the import process doesn't produce a new tree - it operates on the tree started in the root file and recursively and mutatively flattens things on. I'm not entirely sure I like the approach; I'd welcome other suggestions.

tstirrat15 · 2025-01-30T23:07:58Z

pkg/composableschemadsl/compiler/compiler.go

+func parseSchema(schema InputSchema) (*dslNode, input.PositionMapper, error) {
+	mapper := newPositionMapper(schema)
+	root := parser.Parse(createAstNode, schema.Source, schema.SchemaString).(*dslNode)
+	errs := root.FindAll(dslshape.NodeTypeError)
+	if len(errs) > 0 {
+		err := errorNodeToError(errs[0], mapper)
+		return nil, nil, err
+	}
+	return root, mapper, nil
+}


I pulled this out into a separate function because the import functionality parses as a part of its work.

tstirrat15 · 2025-01-30T23:15:23Z

pkg/composableschemadsl/compiler/translator.go

+	compiledPartials    map[string][]*core.Relation
+	partialWaitingRooms *mapz.MultiMap[string, *dslNode]


These are for the partial compilation process. partialWaitingRooms could probably move into the translatePartials function if desired.

why is it called a waiting room?

It was a name i reached for. The idea is that the values of a given key are the partials whose compilation depends on the compilation of the partial named in the key, and my brain went to "waiting room" for that concept. I can change that to "unresolvedPartials" or something if that's preferable.

tstirrat15 · 2025-01-30T23:15:39Z

pkg/composableschemadsl/compiler/translator.go

-		case dslshape.NodeTypeImport:
-			compiled, err := translateImport(tctx, topLevelNode, names)
-			if err != nil {
-				return nil, err


This logic moved into translateImports

tstirrat15 · 2025-01-30T23:17:33Z

pkg/composableschemadsl/compiler/translator.go

+// NOTE: this function and the translatePartial variation differ in how they treat a missing reference.
+// This function treats that as an error state, since all partials should be resolved by that point;
+// the partial variant treats that as a skip.
+func translateDefinitionRelationsAndPermissions(tctx translationContext, astNode *dslNode) ([]*core.Relation, error) {


I imagine there's some deduplication to be done between this and the function below, but the point at which they differ is stuck in the middle. I'd be fine leaving this as a target for future refactoring.

pass a flag and change behavior on that bool

tstirrat15 · 2025-01-30T23:17:55Z

pkg/composableschemadsl/compiler/translator.go

+
+// Takes a parsed schema and recursively translates import syntax and replaces
+// import nodes with parsed nodes from the target files
+func translateImports(itctx importTranslationContext, root *dslNode) error {


This is now where the import logic is happening.

tstirrat15 · 2025-01-30T23:19:35Z

pkg/composableschemadsl/compiler/translator.go

+	if tctx.partialWaitingRooms.Len() != 0 {
+		return fmt.Errorf("could not resolve partials: [%s]. this may indicate a circular reference", strings.Join(tctx.partialWaitingRooms.Keys(), ", "))
 	}


This engenders an assumption that the algorithm using this approach is correct. So far it looks correct, but I don't know that I could prove it right now.

josephschorr · 2025-01-31T18:22:38Z

pkg/composableschemadsl/compiler/compiler_test.go

+			withTenantPrefix,
+			`
+			definition simple {
+				...some_partial


add some tests that have concrete impls next to the partials

josephschorr · 2025-01-31T18:23:05Z

pkg/composableschemadsl/compiler/node.go

@@ -35,6 +35,15 @@ func (tn *dslNode) Connect(predicate string, other parser.AstNode) {
 	tn.children[predicate].PushBack(other)
 }

+// Used to preserve import order when doing import operations on AST
+func (tn *dslNode) ConnectAndHoistMany(predicate string, other *list.List) {


does the order really matter?

It doesn't, but it means that tests pass without regeneration (which makes me feel better about not having missed something) and that the generated output matches my mental model of imports (in terms of imports being replaced by the contents of the referenced files).

Okay. Maybe name "ConnectAtFront"?

I'd be more keen to unwind it in a later refactor if we want to drop it as an optimization

josephschorr · 2025-01-31T18:23:19Z

pkg/composableschemadsl/compiler/translator.go

+	compiledPartials    map[string][]*core.Relation
+	partialWaitingRooms *mapz.MultiMap[string, *dslNode]


why is it called a waiting room?

josephschorr · 2025-01-31T18:23:36Z

pkg/composableschemadsl/compiler/translator.go

+	// Do an initial pass to translate partials and add them to the
+	// translation context. This ensures that they're available for
+	// subsequent reference in definition compilation.
+	err := translatePartials(tctx, root)


call this collectPartials, since it isn't actually translating them into the root of the tree

josephschorr · 2025-01-31T18:24:03Z

pkg/composableschemadsl/compiler/translator.go

+// NOTE: this function and the translatePartial variation differ in how they treat a missing reference.
+// This function treats that as an error state, since all partials should be resolved by that point;
+// the partial variant treats that as a skip.
+func translateDefinitionRelationsAndPermissions(tctx translationContext, astNode *dslNode) ([]*core.Relation, error) {


pass a flag and change behavior on that bool

josephschorr · 2025-01-31T18:24:18Z

pkg/composableschemadsl/compiler/translator.go

-	path, err := importNode.GetString(dslshape.NodeImportPredicatePath)
-	if err != nil {
-		return nil, err
+type importTranslationContext struct {


maybe importResolutionContext

josephschorr

LGTM

josephschorr · 2025-01-31T21:06:22Z

pkg/composableschemadsl/compiler/node.go

@@ -35,6 +35,15 @@ func (tn *dslNode) Connect(predicate string, other parser.AstNode) {
 	tn.children[predicate].PushBack(other)
 }

+// Used to preserve import order when doing import operations on AST
+func (tn *dslNode) ConnectAndHoistMany(predicate string, other *list.List) {


github-actions bot added area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/dependencies Affects dependencies labels Jan 28, 2025

tstirrat15 force-pushed the implement-partials-compilation branch 5 times, most recently from 0cec82e to 9e66052 Compare January 30, 2025 23:04

tstirrat15 marked this pull request as ready for review January 30, 2025 23:04

tstirrat15 requested a review from a team as a code owner January 30, 2025 23:04

github-actions bot removed the area/dependencies Affects dependencies label Jan 30, 2025

tstirrat15 force-pushed the implement-partials-compilation branch 3 times, most recently from c3d13b3 to 9e735d8 Compare January 30, 2025 23:20

tstirrat15 commented Jan 30, 2025

View reviewed changes

josephschorr reviewed Jan 31, 2025

View reviewed changes

tstirrat15 force-pushed the implement-partials-compilation branch from 9e735d8 to 88ff405 Compare January 31, 2025 20:50

Implement imports on AST

5b6840f

tstirrat15 force-pushed the implement-partials-compilation branch from 88ff405 to 5b6840f Compare January 31, 2025 20:51

josephschorr approved these changes Jan 31, 2025

View reviewed changes

tstirrat15 added this pull request to the merge queue Jan 31, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 31, 2025

tstirrat15 added this pull request to the merge queue Jan 31, 2025

Merged via the queue into main with commit f5c4424 Jan 31, 2025
39 checks passed

tstirrat15 deleted the implement-partials-compilation branch January 31, 2025 22:18

github-actions bot locked and limited conversation to collaborators Jan 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement partials compilation #2212

Implement partials compilation #2212

tstirrat15 commented Jan 28, 2025 •

edited

Loading

tstirrat15 left a comment

tstirrat15 Jan 30, 2025

tstirrat15 Jan 30, 2025

tstirrat15 Jan 30, 2025

tstirrat15 Jan 30, 2025

tstirrat15 Jan 30, 2025

tstirrat15 Jan 30, 2025

josephschorr Jan 31, 2025

tstirrat15 Jan 31, 2025

tstirrat15 Jan 30, 2025

tstirrat15 Jan 30, 2025

josephschorr Jan 31, 2025

tstirrat15 Jan 30, 2025

tstirrat15 Jan 30, 2025

josephschorr Jan 31, 2025

josephschorr Jan 31, 2025

tstirrat15 Jan 31, 2025

josephschorr Jan 31, 2025

tstirrat15 Jan 31, 2025

josephschorr Jan 31, 2025

josephschorr Jan 31, 2025

josephschorr Jan 31, 2025

josephschorr Jan 31, 2025

josephschorr Jan 31, 2025

josephschorr left a comment

josephschorr Jan 31, 2025

		compiledPartials map[string][]*core.Relation
		partialWaitingRooms mapz.MultiMap[string, dslNode]

Implement partials compilation #2212

Implement partials compilation #2212

Conversation

tstirrat15 commented Jan 28, 2025 • edited Loading

Description

Changes

Testing

tstirrat15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephschorr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstirrat15 commented Jan 28, 2025 •

edited

Loading